DATAUP-729: job timestamp easy pickings by ialarmedalien · Pull Request #2950 · kbase/narrative

ialarmedalien · 2022-05-04T23:13:09Z

Description of PR purpose/changes

This is a set of "easy pickings" from @n1mus (or Summit, as autocorrect calls her)'s DATAUP-729-job-ts branch. The original PR was large and complex so this updates and merges it into develop (see the first commit), and then picks out a small-ish set of changes for the first PR. It's expected that there will be (at least) two more PRs to cover the rest of the changes.

I ran isort, the import sorter, on the altered files.

Jira Ticket / Issue

Related Jira ticket: https://kbase-jira.atlassian.net/browse/DATAUP-729

Added the Jira Ticket to the title of the PR (e.g. DATAUP-69 Adds a PR template)

Testing Instructions

Details for how to test the PR:

Tests pass locally and in GitHub Actions
Changes available by spinning up a local narrative and seeing that nothing has changed.

Dev Checklist:

My code follows the guidelines at https://sites.google.com/lbl.gov/trussresources/home?authuser=0
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
(Python) I have run Black and Flake8 on changed Python code manually or with a git precommit hook

ialarmedalien · 2022-05-04T23:14:32Z

src/biokbase/narrative/jobs/appmanager.py

 """
 A module for managing apps, specs, requirements, and for starting jobs.
 """
+import datetime


ialarmedalien · 2022-05-04T23:17:06Z

src/biokbase/narrative/jobs/job.py


-class Job(object):
-    _job_logs = list()
+class Job:


no need for (object) in python 3

ialarmedalien · 2022-05-04T23:18:50Z

src/biokbase/narrative/jobs/job.py

-class Job(object):
-    _job_logs = list()
+class Job:
+    _job_logs = []


from the flake8-comprehensions plugin (installed locally)

ialarmedalien · 2022-05-04T23:19:43Z

src/biokbase/narrative/jobs/job.py

        """
-        attr = dict(
-            app_id=lambda: self._acc_state.get("job_input", {}).get(
+        attr = {


flake8-comprehensions again

ialarmedalien · 2022-05-04T23:20:34Z

src/biokbase/narrative/jobs/job.py

+        # Check if there would be no change in updating
+        # i.e., if state <= self._acc_state
+        if self._acc_state is not None:
+            if {**self._acc_state, **state} == self._acc_state:
+                return


prep for upcoming timestamps work

lgtm-com · 2022-05-04T23:20:50Z

This pull request introduces 2 alerts when merging 3dc225d into 213fd59 - view on LGTM.com

new alerts:

2 for Property in old-style class

ialarmedalien · 2022-05-04T23:21:53Z

src/biokbase/narrative/jobs/jobcomm.py


+    @property
+    def ts(self):
+        """This param is completely optional"""


@n1mus is going to write a better description for this param in a forthcoming PR 😁

ialarmedalien · 2022-05-04T23:23:50Z

src/biokbase/narrative/jobs/jobcomm.py

+                MESSAGE_TYPE["CANCEL"]: self.cancel_jobs,
+                MESSAGE_TYPE["CELL_JOB_STATUS"]: self.get_job_states_by_cell_id,
+                MESSAGE_TYPE["INFO"]: self.get_job_info,
+                MESSAGE_TYPE["LOGS"]: self.get_job_logs,
+                MESSAGE_TYPE["RETRY"]: self.retry_jobs,
                MESSAGE_TYPE["START_UPDATE"]: self._modify_job_updates,
-                MESSAGE_TYPE["STATUS"]: self._get_job_states,
-                MESSAGE_TYPE["STATUS_ALL"]: self._get_all_job_states,
+                MESSAGE_TYPE["STATUS"]: self.get_job_states,
+                MESSAGE_TYPE["STATUS_ALL"]: self.get_all_job_states,


renamed these methods for clarity and to make a better distinction between top level and internal methods

Nice! This would always throw me for a loop now that I think of it

ialarmedalien · 2022-05-04T23:26:14Z

src/biokbase/narrative/jobs/jobcomm.py

        return job_info

-    def __get_job_states(self, job_id_list) -> dict:
+    def _get_job_states(self, job_id_list: list, ts: int = None) -> dict:


note new ts parameter

codecov · 2022-05-04T23:27:35Z

Codecov Report

Merging #2950 (3dc225d) into develop (66e5c6e) will increase coverage by 0.05%.
The diff coverage is 98.71%.

❗ Current head 3dc225d differs from pull request most recent head 52e379b. Consider uploading reports for the commit 52e379b to get more accurate results

@@             Coverage Diff             @@
##           develop    #2950      +/-   ##
===========================================
+ Coverage    73.19%   73.25%   +0.05%     
===========================================
  Files           36       36              
  Lines         3895     3903       +8     
===========================================
+ Hits          2851     2859       +8     
  Misses        1044     1044

Impacted Files	Coverage Δ
src/biokbase/narrative/jobs/jobmanager.py	`95.35% <90.90%> (-0.02%)`	⬇️
src/biokbase/narrative/jobs/appmanager.py	`91.86% <100.00%> (-0.31%)`	⬇️
src/biokbase/narrative/jobs/job.py	`90.51% <100.00%> (+0.64%)`	⬆️
src/biokbase/narrative/jobs/jobcomm.py	`98.95% <100.00%> (+0.02%)`	⬆️
src/biokbase/narrative/jobs/util.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcbad86...52e379b. Read the comment docs.

ialarmedalien · 2022-05-04T23:28:00Z

src/biokbase/narrative/jobs/jobmanager.py

-                "return_list": 0,
-            }
-        )
+        job_states = Job.query_ee2_states(job_ids, init=True)


@n1mus has consolidated most of the ee2 queries (apart from the workspace job fetch) to use the query_ee2_state(s) functions.

ialarmedalien · 2022-05-04T23:33:53Z

src/biokbase/narrative/tests/test_job.py

            self.assertEqual(getattr(jobl, attr), getattr(jobr, attr))

-    def check_job_attrs_custom(self, job, exp_attr={}):
+    def check_job_attrs_custom(self, job, exp_attr=None):


don't use mutable args as defaults

Ha I've learned something

ialarmedalien · 2022-05-04T23:34:50Z

src/biokbase/narrative/tests/test_jobcomm.py

-import unittest
-from unittest import mock
-import os
+import copy


isort got busy on this file!

ghost · 2022-05-05T02:55:03Z

src/biokbase/narrative/jobs/appmanager.py

            ret = str(generator["prefix"]) + ret
        if "suffix" in generator:
-            ret = ret + str(generator["suffix"])
+            return ret + str(generator["suffix"])


? No biggie but I thought this was strange

I've got a flake8 plugin installed locally that warns if there's somewhere where a variable can be returned immediately. This is one such case...

ghost · 2022-05-05T02:58:50Z

src/biokbase/narrative/jobs/jobcomm.py


+    @property
+    def ts(self):
+        """This param is completely optional"""


ghost · 2022-05-05T02:59:34Z

src/biokbase/narrative/jobs/jobcomm.py

+                MESSAGE_TYPE["CANCEL"]: self.cancel_jobs,
+                MESSAGE_TYPE["CELL_JOB_STATUS"]: self.get_job_states_by_cell_id,
+                MESSAGE_TYPE["INFO"]: self.get_job_info,
+                MESSAGE_TYPE["LOGS"]: self.get_job_logs,
+                MESSAGE_TYPE["RETRY"]: self.retry_jobs,
                MESSAGE_TYPE["START_UPDATE"]: self._modify_job_updates,
-                MESSAGE_TYPE["STATUS"]: self._get_job_states,
-                MESSAGE_TYPE["STATUS_ALL"]: self._get_all_job_states,
+                MESSAGE_TYPE["STATUS"]: self.get_job_states,
+                MESSAGE_TYPE["STATUS_ALL"]: self.get_all_job_states,


Nice! This would always throw me for a loop now that I think of it

ghost · 2022-05-05T03:03:45Z

src/biokbase/narrative/jobs/jobmanager.py

        try:
            all_states = self.get_all_job_states(ignore_refresh_flag=True)
-            state_list = [s["jobState"] for s in all_states.values()]
+            state_list = [copy.deepcopy(s["jobState"]) for s in all_states.values()]


Shouldn't need this anymore since we fixed the double- yet leaky-caching, but no biggie

would you mind fixing it in the next PR?

ghost · 2022-05-05T03:06:47Z

src/biokbase/narrative/tests/test_job.py

            self.assertEqual(getattr(jobl, attr), getattr(jobr, attr))

-    def check_job_attrs_custom(self, job, exp_attr={}):
+    def check_job_attrs_custom(self, job, exp_attr=None):


Ha I've learned something

… branch

lgtm-com · 2022-05-05T14:00:26Z

This pull request introduces 2 alerts when merging 52e379b into bcbad86 - view on LGTM.com

new alerts:

2 for Property in old-style class

sonarqubecloud · 2022-05-05T14:09:13Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

ialarmedalien requested review from a user and briehl May 4, 2022 23:13

ialarmedalien self-assigned this May 4, 2022

ialarmedalien commented May 4, 2022

View reviewed changes

ghost approved these changes May 5, 2022

View reviewed changes

n1mus and others added 2 commits May 5, 2022 06:53

Merge DATAUP-729-job-ts into develop

07fd057

Cherry-picking out a set of simple changes from the DATAUP-729-job-ts…

52e379b

… branch

ialarmedalien force-pushed the DATAUP-729_job_ts_easy_pickings branch from 3dc225d to 52e379b Compare May 5, 2022 13:54

briehl approved these changes May 6, 2022

View reviewed changes

ialarmedalien merged commit df43735 into develop May 6, 2022

ialarmedalien deleted the DATAUP-729_job_ts_easy_pickings branch May 6, 2022 22:47

Conversation

ialarmedalien commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR purpose/changes

Jira Ticket / Issue

Testing Instructions

Dev Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented May 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented May 5, 2022

Uh oh!

sonarqubecloud bot commented May 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ialarmedalien commented May 4, 2022 •

edited

Loading

codecov bot commented May 4, 2022 •

edited

Loading